continuation of image_ref work #1406

bparees · 2015-03-20T23:25:57Z

make From a pointer

bparees · 2015-03-20T23:39:09Z

bparees · 2015-03-20T23:48:09Z

[test]

bparees · 2015-03-22T22:46:18Z

@mfojtik @smarterclayton there is a fairly ugly consequence to this change... specifically, buildConfigs are defined to prefer their "From" field but use the "Image" field if From is not present. (in fact it's a validation error to supply both, currently). However, Build objects (which use the same STIStrategy type as a field) logically require that you only supply an Image. And even if we allowed you to supply a From field to a Build (on the assumption we'd resolve the From reference as we created the build pod), we'd still have to prefer the Image field since the Image field might have been set earlier by the Tag resolution logic. So it would have the inverse precedence of the BuildConfig.

For now i've documented it as saying that Build objects only use the Image field. I don't really want to create a second STIStrategy type (that only has an Image field), but as things stand now, Build objects are a little funky.

My assumption/justificatin/hope for now is that for the most part people will be creating BuildConfigs, not Builds (especially once we have @soltysh 's new build creation endpoints) and thus people won't be exposed to the Build object semantics.

And i do think it's better that Builds only have Image, not From references, because it makes them more re-runnable. Adding something that's dynamically resolved (like an ImageRepo reference) makes them significantly less immutable.

bparees · 2015-03-23T18:26:36Z

@mfojtik @soltysh ptal. the patterns here are going to be reproduced for Docker and Custom build strategies.

openshift-bot · 2015-03-23T19:40:40Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1431/)

mfojtik · 2015-03-23T19:48:58Z

pkg/build/api/v1beta1/conversion.go

@@ -53,11 +53,25 @@ func init() {
 		func(in *newer.STIBuildStrategy, out *STIBuildStrategy, s conversion.Scope) error {
 			out.BuilderImage = in.Image
 			out.Image = in.Image
+			if in.From != nil {
+				out.From = &kapi.ObjectReference{
+					Name:      in.From.Name,


If this is a ImageRepository reference, can you set the Kind as well?

mfojtik · 2015-03-23T19:50:45Z

pkg/build/util/generate.go

@@ -13,7 +14,8 @@ import (
 // GenerateBuildFromConfig creates a new build based on a given BuildConfig. Optionally a SourceRevision for the new
 // build can be specified.  Also optionally a list of image names to be substituted can be supplied.  Values in the BuildConfig
 // that have a substitution provided will be replaced in the resulting Build
-func GenerateBuildFromConfig(bc *buildapi.BuildConfig, r *buildapi.SourceRevision, imageSubstitutions map[string]string) (build *buildapi.Build) {
+func GenerateBuildFromConfig(bc *buildapi.BuildConfig, r *buildapi.SourceRevision, imageSubstitutions map[string]string,


you know that isn't even something this pull introduced, right? :)

mfojtik · 2015-03-23T19:54:55Z

pkg/build/util/generate.go

+	case config.Parameters.Strategy.Type == buildapi.CustomBuildStrategyType:
+		build, err = GenerateBuildUsingImageTriggerTag(config, revision, imageRepoGetter)
+	default:
+		err = fmt.Errorf("Build strategy type must be set")


why not just return error here?

mfojtik · 2015-03-23T19:56:25Z

pkg/build/util/generate.go

+		tag = buildapi.DefaultImageTag
+	}
+
+	var imageRepo *imageapi.ImageRepository


why declaring these by var? you can just do imageRepo, err :=

mfojtik · 2015-03-23T19:58:14Z

pkg/build/util/generate.go

+		return nil, fmt.Errorf("Docker Image Repository %s has no tag %s", from.Name, tag)
+	}
+	glog.V(4).Infof("Generating build from config for build config %s", config.Name)
+	build := GenerateBuildFromConfig(config, revision, nil, imageRepoSubstitutions)


nuke 'build' var and just return GenerateBuildConfig(), nil

bparees · 2015-03-23T20:28:02Z

fixes #1418

soltysh · 2015-03-23T20:35:19Z

LGTM once it's rebased and passes the tests. Good job @bparees 🎱

bparees · 2015-03-23T20:35:48Z

@soltysh mostly not my work but thanks :/

soltysh · 2015-03-24T11:52:59Z

Just got confirmation from @mfojtik he's OK with it, thus [merge]-ing.

openshift-bot · 2015-03-24T11:55:39Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1268/) (Image: devenv-fedora_1115)

openshift-bot · 2015-03-24T11:55:39Z

Evaluated for origin up to ba587bc

Merged by openshift-bot

smarterclayton · 2015-03-24T17:24:06Z

I've rolled this back because it was randomly failing integration tests and end to end.

smarterclayton · 2015-03-24T17:24:11Z

Please resubmit

smarterclayton · 2015-03-24T17:32:43Z

ok      TestSelfSubjectAccessReviews
failed  TestSimpleConfigChangeTrigger
W0324 13:03:18.122659   18925 master.go:186] Portal net unspecified. Defaulting to 10.0.0.0/24.
I0324 13:03:18.122739   18925 util.go:410] Choosing interface en0 for from-host portals
I0324 13:03:18.122754   18925 util.go:415] Interface en0 = fe80::7a31:c1ff:feb7:6042/64
I0324 13:03:18.122762   18925 master.go:214] Will report fe80::7a31:c1ff:feb7:6042 as public IP address.
I0324 13:03:18.122777   18925 master.go:260] Setting master service IPs based on PortalNet subnet to "10.0.0.1" (read-only) and "10.0.0.2" (read-write).
I0324 13:03:18.160407   18925 controller.go:40] Waiting for first version of integration/change-deploy-config:0
I0324 13:03:18.208057   18925 controller.go:70] Deployment already exists for config integration/change-deploy-config:1
I0324 13:03:18.210811   18925 controller.go:55] Created initial deployment for config integration/change-deploy-config:0
I0324 13:03:18.250687   18925 controller.go:75] Ignoring config change for integration/change-deploy-config:1 (latestVersion=1); same as deployment integration/change-deploy-config-1
I0324 13:03:18.319873   18925 controller.go:86] Updated config integration/change-deploy-config:1 from version 1 to 2 for existing deployment integration/change-deploy-config-1
I0324 13:03:18.321995   18925 controller.go:63] Ignoring config change for integration/change-deploy-config:2; no existing deployment found
--- FAIL: TestSimpleConfigChangeTrigger (0.23s)
    deploy_trigger_test.go:325: Starting test openshift
    deploy_trigger_test.go:285: expected watch event type ADDED, got MODIFIED
FAIL
failed  TestSimpleImageChangeBuildTrigger
W0324 13:03:18.554320   18932 master.go:186] Portal net unspecified. Defaulting to 10.0.0.0/24.
I0324 13:03:18.554439   18932 util.go:410] Choosing interface en0 for from-host portals
I0324 13:03:18.554462   18932 util.go:415] Interface en0 = fe80::7a31:c1ff:feb7:6042/64
I0324 13:03:18.554474   18932 master.go:214] Will report fe80::7a31:c1ff:feb7:6042 as public IP address.
I0324 13:03:18.554491   18932 master.go:260] Setting master service IPs based on PortalNet subnet to "10.0.0.1" (read-only) and "10.0.0.2" (read-write).
I0324 13:03:18.604712   18932 image_change_controller.go:39] Build image change controller detected imagerepo change registry:8080/openshift/test-image-trigger
I0324 13:03:18.604726   18932 image_change_controller.go:83] Running build for buildConfig test-build-cfg in namespace integration
I0324 13:03:18.605470   18932 generate.go:39] Substituting registry:8080/openshift/test-image-trigger:latest for registry:8080/openshift/test-image-trigger
I0324 13:03:18.606063   18932 controller.go:101] Updated all configs for trigger on imageRepo integration/test-image-trigger-repo
I0324 13:03:18.610350   18932 controller.go:42] Handling build test-build-cfg-ab46c96d-d247-11e4-a9ec-7831c1b76042
--- FAIL: TestSimpleImageChangeBuildTrigger (0.12s)
    deploy_trigger_test.go:325: Starting test openshift
    imagechange_buildtrigger_test.go:72: expected watch event type MODIFIED, got ADDED
FAIL
failed  TestSimpleImageChangeTrigger
W0324 13:03:18.709405   18939 master.go:186] Portal net unspecified. Defaulting to 10.0.0.0/24.
I0324 13:03:18.709488   18939 util.go:410] Choosing interface en0 for from-host portals
I0324 13:03:18.709502   18939 util.go:415] Interface en0 = fe80::7a31:c1ff:feb7:6042/64
I0324 13:03:18.709510   18939 master.go:214] Will report fe80::7a31:c1ff:feb7:6042 as public IP address.
I0324 13:03:18.709525   18939 master.go:260] Setting master service IPs based on PortalNet subnet to "10.0.0.1" (read-only) and "10.0.0.2" (read-write).
I0324 13:03:18.743417   18939 image_change_controller.go:39] Build image change controller detected imagerepo change registry:8080/openshift/test-image
I0324 13:03:18.745286   18939 controller.go:101] Updated all configs for trigger on imageRepo integration/test-image-repo
I0324 13:03:18.756752   18939 controller.go:40] Waiting for first version of integration/image-deploy-config:0
I0324 13:03:18.757980   18939 controller.go:43] Ignoring config integration/image-deploy-config:0; no change triggers detected
I0324 13:03:18.778111   18939 controller.go:43] Ignoring config integration/image-deploy-config:1; no change triggers detected
I0324 13:03:18.803168   18939 image_change_controller.go:39] Build image change controller detected imagerepo change registry:8080/openshift/test-image
I0324 13:03:18.805002   18939 controller.go:38] Detecting changed images for deploymentConfig integration/image-deploy-config:1
I0324 13:03:18.805025   18939 controller.go:48] Found matching ImageChange trigger for deploymentConfig integration/image-deploy-config:1: &api.DeploymentTriggerImageChangeParams{Automatic:true, ContainerNames:[]string{"container-1"}, RepositoryName:"registry:8080/openshift/test-image", From:api.ObjectReference{Kind:"", Namespace:"", Name:"", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}, Tag:"latest"}
I0324 13:03:18.805076   18939 controller.go:54] Processing image triggers for deploymentConfig integration/image-deploy-config:1
I0324 13:03:18.805098   18939 controller.go:79] Container container-1 for config integration/image-deploy-config:1: image id changed from "ref-1" to ""; regenerating config
I0324 13:03:18.866738   18939 controller.go:43] Ignoring config integration/image-deploy-config:2; no change triggers detected
I0324 13:03:18.870061   18939 controller.go:94] Updated deploymentConfig integration/image-deploy-config:1 in response to image change trigger
I0324 13:03:18.870071   18939 controller.go:101] Updated all configs for trigger on imageRepo integration/test-image-repo
--- FAIL: TestSimpleImageChangeTrigger (0.19s)
    deploy_trigger_test.go:325: Starting test openshift
    deploy_trigger_test.go:159: expected watch event type ADDED, got MODIFIED
FAIL
ok      TestSimpleImageChangeTriggerFrom

was seen local, and the last two merge e2e's failed (after this landed)

bparees · 2015-03-24T17:51:35Z

only one merge e2e failed since this landed and it was due to the DB pod not starting.

smarterclayton · 2015-04-04T23:45:08Z

When is this going to be added to Docker and Custom build strategies? What card / pull tracks that work?

bparees · 2015-04-05T00:50:53Z

There's a card on the next list, it's high on my priorities but I was waiting for the imagestream changes to land so I didn't have to deal with a bunch of rebases. Hoping to start it this week but I also have the PHP image on my list and a variety of other tidbits.

Ben Parees | OpenShift

From: Clayton Coleman notifications@github.com
Sent: Apr 4, 2015 7:45 PM
To: openshift/origin
Cc: Ben Parees
Subject: Re: [origin] continuation of image_ref work (#1406)

When is this going to be added to Docker and Custom build strategies? What card / pull tracks that work?

Reply to this email directly or view it on GitHub:
#1406 (comment)

smarterclayton · 2015-04-05T02:09:13Z

K. with the status stuff I'm deep in the guts of using the objects programmatically and I'm seeing the faults.

On Apr 4, 2015, at 8:50 PM, Ben Parees notifications@github.com wrote:

There's a card on the next list, it's high on my priorities but I was waiting for the imagestream changes to land so I didn't have to deal with a bunch of rebases. Hoping to start it this week but I also have the PHP image on my list and a variety of other tidbits.

Ben Parees | OpenShift

From: Clayton Coleman notifications@github.com
Sent: Apr 4, 2015 7:45 PM
To: openshift/origin
Cc: Ben Parees
Subject: Re: [origin] continuation of image_ref work (#1406)

When is this going to be added to Docker and Custom build strategies? What card / pull tracks that work?

Reply to this email directly or view it on GitHub:
#1406 (comment)
—
Reply to this email directly or view it on GitHub.

bparees · 2015-04-05T03:17:59Z

Seeing which faults?

Ben Parees | OpenShift

From: Clayton Coleman notifications@github.com
Sent: Apr 4, 2015 10:09 PM
To: openshift/origin
Cc: Ben Parees
Subject: Re: [origin] continuation of image_ref work (#1406)

K. with the status stuff I'm deep in the guts of using the objects programmatically and I'm seeing the faults.

On Apr 4, 2015, at 8:50 PM, Ben Parees notifications@github.com wrote:

There's a card on the next list, it's high on my priorities but I was waiting for the imagestream changes to land so I didn't have to deal with a bunch of rebases. Hoping to start it this week but I also have the PHP image on my list and a variety of other tidbits.

Ben Parees | OpenShift

From: Clayton Coleman notifications@github.com
Sent: Apr 4, 2015 7:45 PM
To: openshift/origin
Cc: Ben Parees
Subject: Re: [origin] continuation of image_ref work (#1406)

When is this going to be added to Docker and Custom build strategies? What card / pull tracks that work?

Reply to this email directly or view it on GitHub:
#1406 (comment)
—
Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub:
#1406 (comment)

smarterclayton · 2015-04-05T03:27:04Z

In the approachability and usability of the APIs. Same stuff we were discussing last week, difficult to write clean logic for both triggers and builds (and then working with deployments and having opposite behavior for triggers now). The objects are complex, but we don't have common iteration logic.

I'm thinking our trigger object references should be to ImageStreamTags now - which would remove the need for the separate tag field. We might also want to fold docker image references directly into a virtual Kind and treat them as object references. All things we should discuss as part of our plan for v1.

On Apr 4, 2015, at 11:18 PM, Ben Parees notifications@github.com wrote:

Seeing which faults?

Ben Parees | OpenShift

From: Clayton Coleman notifications@github.com
Sent: Apr 4, 2015 10:09 PM
To: openshift/origin
Cc: Ben Parees
Subject: Re: [origin] continuation of image_ref work (#1406)

K. with the status stuff I'm deep in the guts of using the objects programmatically and I'm seeing the faults.

On Apr 4, 2015, at 8:50 PM, Ben Parees notifications@github.com wrote:

There's a card on the next list, it's high on my priorities but I was waiting for the imagestream changes to land so I didn't have to deal with a bunch of rebases. Hoping to start it this week but I also have the PHP image on my list and a variety of other tidbits.

Ben Parees | OpenShift

From: Clayton Coleman notifications@github.com
Sent: Apr 4, 2015 7:45 PM
To: openshift/origin
Cc: Ben Parees
Subject: Re: [origin] continuation of image_ref work (#1406)

When is this going to be added to Docker and Custom build strategies? What card / pull tracks that work?

Reply to this email directly or view it on GitHub:
#1406 (comment)
—
Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub:
#1406 (comment)
—
Reply to this email directly or view it on GitHub.

…service-catalog/' changes from 3aacfedec6..aa27078754 aa27078754 origin build: add origin tooling bcf37fd 0.1.0-rc2 chart updates (openshift#1410) 4ab0a0a add back 'Processing' message for instance deletion (openshift#1332) 0ecbcb1 Update logs for Cluster service plans. (openshift#1389) 8b491ef Fix a quoting nit (openshift#1400) 63685e4 add orphan mitigation-specific conditions for instances (openshift#1378) adee662 Updated missed fields in service and plan specs (openshift#1406) 2095919 Handle default plan setting when using k8s names (openshift#1405) 607ba66 Document rbacEnable. (openshift#1404) 268294e Adding rbac definition for v1 api endpoint. (openshift#1284) 103288d differentiate between failed updates and provisions during deletion (openshift#1383) eba8ba4 enable API aggregation and Service Catalog RBAC on Jenkins (openshift#1333) 5a93315 Validate relistDuration is non-negative (openshift#1395) e279d21 Fix log messages for secrets (openshift#1385) 87fa8c9 fix status update when starting orphan mitigation (openshift#1372) 11f18f3 Switch to wget for integration apiserver checks (openshift#1384) 8c44a7d update OSB client to 2.13 (openshift#1392) e64bbd1 default plan admission controller: filter list of service plans/service classes by the class name (openshift#1351) 6648c0e Check field names. Fix issue 1291 (openshift#1379) 5319841 update comment for instance generation check (openshift#1382) 7d5823f remove internal poll method (openshift#1381) 07d3068 Rework the logging for controller_instance. (openshift#1371) 5f4ca01 address PR comment as a followup (openshift#1380) 485d5e6 Add support for specifying plan using K8S names. (openshift#1377) 662bba8 Log number of secret keys created for binding credential (openshift#1375) 8ad6a31 Move controller constants into correct files (openshift#1373) 7bd66dd Adding type to log. (openshift#1339) 1ce5c4d Remove k8s/k8s dependency (openshift#1355) b458323 Adding log formatting for BindingController. (openshift#1352) 275eb11 rename test variables to be consistent (openshift#1315) ffd6b8b travis: skip cleanup before deploy (openshift#1368) d5ecc04 fix travis tag checker (openshift#1365) 2cae0ee Minor updates to README (openshift#1360) REVERT: 3aacfedec6 carry: Set external plan name for service-catalog walkthrough REVERT: 3ec9e5b07a origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: aa2707875461dd51be3731b1d94b5cfc3b9a3976

bparees force-pushed the image_refs branch 4 times, most recently from 6b3a19b to a3efca1 Compare March 20, 2015 23:47

bparees force-pushed the image_refs branch from 26065db to ac7699a Compare March 23, 2015 17:48

bparees changed the title ~~[WIP] continuation of image_ref work~~ continuation of image_ref work Mar 23, 2015

bparees force-pushed the image_refs branch 5 times, most recently from 3a73824 to 9c32817 Compare March 23, 2015 18:19